Add no subscribers fallback option to GoChannel Pub/Sub #418
Conversation
pubsub/gochannel/pubsub.go
Outdated
| @@ -11,6 +11,10 @@ import ( | |||
| "github.com/ThreeDotsLabs/watermill/message" | |||
| ) | |||
|
|
|||
| // NoSubscribersFallbackTopic is the fallback topic messages without any subscribers will be sent to. | |||
| // This is used if the `EnableFallback` configuration option is enabled. | |||
| const NoSubscribersFallbackTopic = "*" | |||
There was a problem hiding this comment.
what do you think about adding configuration for the topic name instead? so we could have two options in Config
EnableFallback
NoSubscribersFallbackTopic
what do you think?
There was a problem hiding this comment.
I added both EnableNoSubscribersFallback (as per your review comment below) and NoSubscribersFallbackTopic to the configuration. I did want to leave the default topic (*) as a const in case the user doesn't set a custom fallback topic. I also added another test case for this.
pubsub/gochannel/pubsub.go
Outdated
|
|
||
| // When true, messages sent to a topic without any subscribers will be sent to the | ||
| // subscribers of the `*` topic. | ||
| EnableFallback bool |
There was a problem hiding this comment.
maybe it would be a bit longer, IMO it could be a bit more explicit, like EnableNoSubscribersFallback
There was a problem hiding this comment.
Yup, I agree that the longer version is more expressive and changed it.
pubsub/gochannel/pubsub.go
Outdated
|
|
||
| // When true, messages sent to a topic without any subscribers will be sent to the | ||
| // subscribers of the `*` topic. | ||
| EnableFallback bool |
There was a problem hiding this comment.
also, food for thought, but I wonder if this fallback couldn't be a closure 🤔
like NoSubscribersFallback func(msg message.Message, pubsub *GoChannel) error
It could be much more flexible 🤔 WDYT?
There was a problem hiding this comment.
That sounds worthwhile to investigate.
There was a problem hiding this comment.
I investigated this with a private project of mine where I'm using the fork and the closure is awkward to use because the function you want to call has to be known and available when instantiating the GoChannel. However, I'm passing this GoChannel object around for subscribing and publishing and then just use
noSubscribersMessages, err := pubSub.Subscribe(ctx, gochannel.NoSubscribersFallbackDefaultTopic)
if err != nil {
return fmt.Errorf("subscribing to fallback topic: %w", err)
}
go func(ctx context.Context, messages <-chan *message.Message) {
for {
select {
case msg := <-messages:
// handle message
// lastly:
msg.Ack()
case <-ctx.Done():
return
}
}
}(ctx, noSubscribersMessages)which works well and is easy to use. That is to say, I'd leave this as is.
|
Just saw your review – thank you! I’ll take a closer look next week. |
If the option is enabled, messages sent to topic without any subscribers will be forwarded to the `*` topic subscribers.
This goes back to the simple if and an early return when the option isn't enabled.
annismckenzie
force-pushed
the
add-no-subscribers-fallback-to-gochannel-pubsub
branch
from
7fa1b92
to
c2ff17b
Compare
|
Short update: I rebased and addressed your comments, see above. I also updated a couple comments as promised in the pull request description in 8808fd7 and this would be ready for another look. |
I needed a way to listen to any message sent via the GoChannel Pub/Sub that didn't have a subscriber yet. This was mainly used for debugging but it could be useful for other purposes as well.
I made sure to add a test and left the rest of the tests untouched – the test suite is passing.
Would you be open to accepting this contribution? If so, I'd take another pass over the current implementation's comments, some would need updating (for example,
That means if you send a message to a topic to which no subscriber is subscribed, that message will be discarded.– update: this is done ☑️).